src/main/resources/templates/log/file.html (1)

50-50: Move inline button styles into the existing component CSS.

Inlined style="width: auto; padding: 8px 16px;" on the submit button mixes presentation into the template and is duplicated in log/comment.html (line 50). Prefer a small modifier class (e.g., btn-submit btn-inline) defined in components.css.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/templates/log/file.html` at line 50, Remove the inline
style on the submit button element and replace it with a small modifier class
(e.g., add "btn-inline" alongside "btn-submit" on the <button> in the template),
then add the corresponding CSS rule in the shared component stylesheet
(components.css) like .btn-inline { width: auto; padding: 8px 16px; } so
presentation is centralized; apply the same change to the duplicate button in
log/comment.html to avoid duplication.

src/main/resources/templates/log/comment.html (1)

1-123: Consider extracting a shared log-page Thymeleaf fragment.

log/comment.html and log/file.html are structurally identical: same header/count, same filter form, same pager, same empty state, with only the action URL, table columns, and event-type field differing. As more log views are added (or the existing ones evolve — pager styling, accessibility, sort controls, etc.), they will drift. A fragments/log-page.html fragment parameterized by the action URL, column definitions, and event-type field would remove the duplication.

Not blocking — the current template renders correctly and pagination/filter state is preserved.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/templates/log/comment.html` around lines 1 - 123, Extract
the repeated page shell (header, log-count, filter form, pager and empty-state)
from log/comment.html and log/file.html into a parameterized Thymeleaf fragment
e.g. fragments/log-page.html; expose parameters for the form action URL, the
eventTypes/selectedEventType bindings, the date field names/values (from/to),
the logs Page object, and the table body/header insertion point. Replace the
duplicated markup in log/comment.html and log/file.html with a single th:replace
call to fragments/log-page.html, passing the specific action URL and a fragment
(or markup) for the table columns/rows and event-type select so each page
supplies only its differing pieces while the common pagination/filtering logic
lives in the fragment.

src/test/java/org/example/visacasemanagementsystem/audit/service/CommentLogServiceTest.java (2)

199-199: Nit: nullable(SingularAttribute.class) is broader than needed.

root.get(...) is invoked here with a SingularAttribute (via the JPA static metamodel CommentLog_), not with a null argument. any(SingularAttribute.class) (which excludes nulls) would more precisely express the expectation; nullable(...) is typically reserved for matchers that must also accept null. Same nit applies at lines 234, 269, and 317. Cosmetic only.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/audit/service/CommentLogServiceTest.java`
at line 199, Replace the broader nullable matcher with a non-null matcher where
the test expects a real SingularAttribute: change usages of
nullable(SingularAttribute.class) in the CommentLogServiceTest Mockito stubbings
(the when(root.get(...)) calls) to any(SingularAttribute.class) so the matcher
more precisely reflects that CommentLog_.<...> attributes (not null) are passed;
apply the same update to the other occurrences mentioned (the other
when(root.get(...)) stubbings in the test).

---

280-330: Composition assertion is tightly coupled to evaluation order — consider loosening or guarding.

The chained cb.and(eqPred, gtePred) → and1 and cb.and(and1, ltePred) → and2 stubs (Lines 321–322) replicate the production code's exact predicate-combining order. The inline comment already calls this out, which is good. However, Specification.and(...) is a default method implementation of Specification in Spring Data JPA; if that internal composition strategy ever changes (e.g. to flatten into a single cb.and(p1, p2, p3) varargs call, which cb.and supports), every "all filters" test in this file will silently break even though the production logic remains semantically equivalent.

Two lighter-touch options if you want to keep this future-proof without losing intent:

• Assert only the three leaf predicate calls (cb.equal, cb.greaterThanOrEqualTo, cb.lessThanOrEqualTo) plus that actual is non-null, and drop the cb.and(...) chaining stubs.
• Or use verify(cb, atLeastOnce()).and(any(Predicate.class), any(Predicate.class)) to assert that AND-composition occurred without pinning the exact pairing tree.

Not blocking — the comment makes the coupling explicit, and the same pattern is mirrored in FileLogServiceTest.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/audit/service/CommentLogServiceTest.java`
around lines 280 - 330, The test tightly couples to the exact
predicate-composition order by stubbing cb.and(eqPred, gtePred) -> and1 and
cb.and(and1, ltePred) -> and2 when capturing the Specification in
CommentLogServiceTest.findFiltered_shouldComposeAllPredicates_WhenAllFiltersAreSet;
loosen this by removing the cb.and(...) chaining stubs and either (a) assert the
three leaf predicate calls (verify cb.equal(eventTypePath,...),
cb.greaterThanOrEqualTo(...), cb.lessThanOrEqualTo(...)) and that
specCaptor.getValue().toPredicate(...) returns non-null, or (b) replace the
strict and stubs with a more permissive verify like verify(cb,
atLeastOnce()).and(any(Predicate.class), any(Predicate.class)) so the test
checks that AND-composition occurred without depending on the internal tree
shape of Specification.and.

src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java (1)

138-145: Minor: pageTitle model attribute is set but not asserted in the new tests.

renderLogPage adds pageTitle (e.g. "Comment Log", "File Log") to the model, but LogViewControllerTest doesn't assert it for any endpoint (existing or new). If the templates rely on ${pageTitle} for the page heading or <title>, a regression that drops it from the controller would slip through web-layer tests. Consider adding model().attribute("pageTitle", "Comment Log") (and similar) to the existing default-render tests, or accept the gap as covered by template/integration tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java`
around lines 138 - 145, The controller method renderLogPage adds a "pageTitle"
model attribute but tests in LogViewControllerTest do not assert it; update the
web-layer tests that exercise renderLogPage (e.g., the tests that hit endpoints
which call renderLogPage) to include assertions like
model().attribute("pageTitle", "<expected title>") for each endpoint (for
example "Comment Log" and "File Log") so the model contract is verified; locate
the controller method renderLogPage and the corresponding test methods in
LogViewControllerTest and add the model attribute assertions with the exact
expected strings.

src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java (1)

338-429: Optional: asymmetric paging-edge coverage across the four endpoints.

Pagination clamping (MAX_PAGE_SIZE, negative page/size minima) lives in a single shared helper (buildPageable in LogViewController), so per-endpoint duplication isn't required for correctness. That said, the matrix is currently uneven:

Endpoint	NoFilters	Filters	ExplicitPaging	OversizedSize	NegativePaging
/log/visa	✅	✅	✅	✅	✅
/log/user	✅	✅	—	—	—
/log/comment	✅	✅	—	✅	—
/log/file	✅	✅	✅	—	—

If you want endpoint-symmetric guarantees (e.g. catching a future regression where a new handler stops calling buildPageable), add the missing oversized/negative cases. Otherwise relying on the /log/visa cases as the canonical clamping contract is fine — this is a code-organization choice, not a defect.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java`
around lines 338 - 429, The test coverage is asymmetric for paging edge cases;
add tests that assert buildPageable clamping/negative handling for the remaining
endpoints by exercising /log/user, /log/comment, and /log/file with oversized
size and negative page/size params and verifying the corresponding
service.findFiltered calls receive the clamped Pageable; specifically add tests
similar to commentLog_AsSysadmin_WithOversizedPageSize_ShouldClampToMax and the
visa negative-page/size tests but targeting UserLogController methods (userLog
handler -> userLogService.findFiltered), CommentLogController handler
(commentLogService.findFiltered) and FileLogController handler
(fileLogService.findFiltered), using mockMvc.perform(get(...).param(...)) and
eq(PageRequest.of(..., clampedOrMinSize, Sort.by(...))) in verify to assert the
shared buildPageable behavior.

src/test/java/org/example/visacasemanagementsystem/audit/service/FileLogServiceTest.java (1)

1-335: Heavy duplication with CommentLogServiceTest — consider an abstract base or parameterized helper later.

This file is structurally identical to CommentLogServiceTest.java (same filter scenarios, same mocked Criteria API plumbing, same ArgumentCaptor patterns), differing only in entity/event-type names and DTO record shape. As more *LogServiceTest classes accrete, an abstract AbstractLogServiceFilteredTest<E extends Enum<E>, ENTITY, DTO> could host the four findFiltered_* cases and the createX_shouldPassSameEntityInstance case. Not blocking for this PR — flag it for a follow-up cleanup after Visa/User log services adopt the same findFiltered pattern.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/audit/service/FileLogServiceTest.java`
around lines 1 - 335, Tests in FileLogServiceTest duplicate
CommentLogServiceTest; extract common filtered-spec and instance-passing tests
into a generic base test class and reuse it. Create an abstract
AbstractLogServiceFilteredTest<E, ENTITY, DTO> that contains the findFiltered_*
tests (the ones invoking FileLogService#findFiltered behavior and capturing
Specification) and the createFileLog_shouldPassSameEntityInstance check,
parameterize it by enum/event type, entity and DTO, and have FileLogServiceTest
and CommentLogServiceTest extend it, overriding/implementing factory methods or
fields for the service under test, mapper, repository, example entity/DTO and
the enum constant; update the concrete tests to only include service-specific
cases not covered by the base class.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes